Bug-hunt round 33: unbounded total-size JSONL reads, two nitpicks - #49
Merged
Conversation
ReadJSONL capped a single line at MaxJSONLLine but the whole file was unbounded, contradicting the comment on maxManifestBytes that lists it as an already-bounded sibling reader. A session's JSONL artefacts are attacker-controllable when exchanged, and a file built from many small, individually-legal lines defeated the per-line cap while driving json.Unmarshal's per-line allocation well past the bytes on disk, OOMing merge, report, and analyze. Add maxJSONLBytes (16 MiB, matching analyze.Ingest's existing cap for untrusted input at the same scale) as a running-total check in ReadJSONL, and a matching pre-flight check in WriteJSONL so a set of records that ReadJSONL could not read back is refused before the file is opened, preserving the existing write-before-read invariant. Assisted-by: Claude:claude-sonnet-5
eventLine's parts always starts with mdOrDash(raw("kind")), which is
at minimum the "—" placeholder, so the join it feeds into orDash can
never be empty and the wrapper's fallback branch is unreachable
(66.7% coverage on a 3-statement function). orDash had no other call
site.
Assisted-by: Claude:claude-sonnet-5
AGENTS.md's "Build, test, and checks" section claims CI runs "every gate above" except the illustrative single-test example, but .github/workflows/ci.yml only runs `go test -race ./...`, superseding the plain `go test ./...` line listed just above it. No test is race-conditional or skipped between the two, so this is a wording fix, not a coverage gap. CLAUDE.md is a symlink to this file. Assisted-by: Claude:claude-sonnet-5
Two 2026-07-18 entries (finding-field sanitisation, the confirmation- hunt hardening pass) were spliced in ahead of a run of 2026-07-17 entries they were committed after, violating the file's own "newest last" rule by both date and commit order. Move them back after the 2026-07-17 block they were inserted into. Assisted-by: Claude:claude-sonnet-5
Assisted-by: Claude:claude-sonnet-5
Round 28 already considered and explicitly rejected removing this wrapper for the same reachability argument this round re-raised: it is a deliberate, locally-redundant guard against a future caller invariant change, the same rationale review.go's checkTargets states for its own SafeText calls (recorded in .abcd/work/DECISIONS.md, 2026-08-05). An adversarial PR reviewer caught the reintroduction before merge. Assisted-by: Claude:claude-sonnet-5
Adversarial PR review (both correctness and docs-accuracy lenses) caught that the prior commit's own comment overclaimed: findings.jsonl is read by analyze.ParseRecords, a separate scanner from session.ReadJSONL, and it carried no total-size bound — the same unbounded-total-file class of defect this round set out to close, just in a second reader. Export session.MaxJSONLBytes (was maxJSONLBytes) so ParseRecords can enforce the same running-total cap ReadJSONL now does. Also corrects three now-inaccurate comments the reviewers flagged: maxManifestBytes's sibling-reader claim named session.ReadJSONL alone as bounding every untrusted JSONL file, when findings.jsonl never passes through it; WriteJSONL's invariant comment named findings.jsonl as one of its callers, but WriteJSONL has exactly two call sites (transcript.jsonl, timeline.jsonl) — findings.jsonl is written by analyze.commitFindings and review.AppendVerdict through their own locked descriptors; and a CRLF file undercounts ReadJSONL's running total by one byte per line (benign — the extra byte is never decoded or retained — but the comment claimed exact counting). Also speeds up the two new session tests introduced for the total-size cap: padding lines/records to a few KB each cuts the oversized-total fixtures from ~2.1M elements to a few thousand, an 8x race-mode speedup on the package (measured 25.7s -> ~4s). Assisted-by: Claude:claude-sonnet-5
Assisted-by: Claude:claude-sonnet-5
REPPL
changed the title
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 33 of the autonomous bug-hunt loop (state tracked on #24).
Confirmed findings
Substantive (1 defect, two readers)
session.ReadJSONLbounds a single line but never the whole file —internal/session/session.go(sc.Buffer(..., MaxJSONLLine)caps one line; the decoded slice is unbounded across lines), contradicting the original comment onmaxManifestBytes, which listedReadJSONLas an already-bounded sibling reader.A session's JSONL artefacts (
timeline.jsonl,interactions.jsonl,transcript.jsonl,findings.jsonl) are attacker-controllable once a session is exchanged (the codebase's own stated threat model, e.g.internal/report/report.go:23,internal/session/session.go). A file built from many small, individually-legal lines defeats the per-line cap whilejson.Unmarshal's per-line allocation runs well past the bytes on disk — measured at roughly 14x amplification, so a file in the low hundreds of MB reliably OOMsmerge,report, andanalyzeunder an ordinary memory ceiling, exiting 2 (the code reserved for usage errors) instead of a clean refusal.Fix: a new
session.MaxJSONLBytes(16 MiB, matchinganalyze.Ingest's existing cap for untrusted input at the same scale) bounds the running total inReadJSONL, with a matching pre-flight check inWriteJSONLfor its two actual callers (transcript.jsonl,timeline.jsonl).Adversarial review of this round's own PR caught a second instance before merge.
findings.jsonlis read byanalyze.ParseRecords, a separate scanner never routed throughsession.ReadJSONL— the round's own fix comment claimed every sibling reader was already bounded, which was false for this one.ParseRecordsnow enforcessession.MaxJSONLBytestoo, and the misleading comments onmaxManifestBytesandWriteJSONLwere corrected to name the actual readers/writers (WriteJSONLnever writesfindings.jsonl;analyze.commitFindings/review.AppendVerdictdo, through their own locked descriptors).New tests:
TestReadJSONLRefusesOversizedTotal,TestReadJSONLAcceptsOrdinaryTotal,TestWriteJSONLRefusesOversizedTotal,TestLoadRejectsOversizedTotal(all confirmed to fail before their respective fix and pass after).Nitpicks (2)
AGENTS.mdtest-gate wording —AGENTS.mdvs.github/workflows/ci.yml.AGENTS.mdlistedgo test ./...andgo test -race ./...as two gates CI runs; CI only runs the race-enabled line. No test differs between the two, so this was a wording fix, not a coverage gap.CLAUDE.mdis a symlink toAGENTS.md.DECISIONS.mdappend-order violation —.abcd/work/DECISIONS.md. Two 2026-07-18 entries had been spliced ahead of a run of 2026-07-17 entries they were actually committed after (confirmed viagit log -Son the entries), breaking the file's own "newest last" rule by both date and commit order. Moved back into place.Considered and reverted before merge
report.eventLine'sorDashwrapper as dead code. Initially flagged and fixed as unreachable (parts[0]is always non-empty, so the fallback branch never fires). An adversarial PR reviewer caught that round 28 had already considered and explicitly rejected this exact claim, recorded in.abcd/work/DECISIONS.md: the wrapper is a deliberate, locally-redundant guard against a future caller invariant change, the same rationalereview.go'scheckTargetsstates for its own SafeText calls. Reverted; the wrapper stays.Considered and rejected
analyze.Ingest"defeating"maxAnswerBytes(internal/analyze/ingest.go). Split refuter verdict: one refuter reproduced the claimed OOM; the other showed the identical OOM reproduces fromjson.Unmarshalalone, before a single validation error accumulates, and that a realistic degenerate LLM answer stays around 255 MB at the 16 MiB cap — well within bounds. The causal claim didn't survive; discarded per the loop's "when in doubt, discard" rule.AGENTS.md's abcd-managed fence (referencing two non-existent paths). Both paths genuinely don't exist, but the two refuters split on actionability (an indirect fix via.abcd/rules.jsonvs. out-of-scope upstream artefact). Discarded on the split.Verification
go build,gofmt -l .,go vet ./...,go test ./...,go test -race ./...all clean; pipeline smoke (merge+reportagainst a scratch copy ofexamples/sample-session) succeeded;sh -n install.sh && bash -n install.shclean. Working tree otherwise untouched.Assisted-by: Claude:claude-sonnet-5